Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-Generated Stubs for Python Bindings #2028

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

joaovbs96
Copy link

@joaovbs96 joaovbs96 commented Sep 27, 2024

PR for #2024

  • Added new bool build option MATERIALX_PYTHON_STUBS to enable/disable automated build of Python stubs through mypy stubgen
  • Modified python/CMakeLists.txt to account for the logic, if the stubgen executable can be found and if the option is turned on
  • Modified pyproject.toml to add mypy as a build dependency and turn on the automated stubs building

Currently, this is still a WIP/Draft. When building with a command such as:

cmake -S . -B build -DMATERIALX_BUILD_PYTHON=ON -DMATERIALX_PYTHON_STUBS=ON -DMATERIALX_BUILD_VIEWER=OFF -DMATERIALX_BUILD_GRAPH_EDITOR=OFF -DMATERIALX_BUILD_TESTS=ON -DMATERIALX_TEST_RENDER=OFF -DMATERIALX_WARNINGS_AS_ERRORS=ON -G "Visual Studio 17 2022" -A "x64"; cmake --build build --target install --config Release --parallel 2

the stubs can be found and work properly in the MaterialX installation. However, when building through pip, such as pip install ., it seems like it doesn't find some folder or file - I'm assuming the envvars are slightly different when building through pip, and I'm currently not taking that into account. Any help would be appreciated!

Copy link

linux-foundation-easycla bot commented Sep 27, 2024

CLA Not Signed

@joaovbs96
Copy link
Author

Seems like, when building through pip install . the execute_process function isn't able to find a file and/or folder. I can guarantee it's currently finding the stubgen executable, so my guess is that the issue is actually regarding the folder where the build files are located.

The stubgen command needs to be executed from where the .pyd files are. For that, I currently set the WORKING_DIRECTORY option as \"${CMAKE_INSTALL_PREFIX}/python\" but I'm guessing this folder just doesn't exist when building through PIP? From the build log, the "install" location of the library files is different and doesn't follow the CMAKE_INSTALL_PREFIX, but I'm not sure what variable to use, then.

)
endif ()
endif()

if(MATERIALX_INSTALL_PYTHON AND PYTHON_EXECUTABLE AND NOT SKBUILD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to move this before stubgen, so it get's run after install.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However to include the stubs in the package itself, You need to repackage.

I think the least number of steps is

  1. run this install step to produce the locally installed MaterialX package.
  2. run your stubgen logic to get the files output to the local MaterialX folder.
  3. run this step again.

I think this should cover locally packaged plus packages generated via SKBUILD. ( I think think the wheels logic is run afterwards as an additional workflow in github actions ).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part do you mean exactly? Or the whole code block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for lack of clarity:

  • You need this if block to run to install.
  • Then you need your entire stub logic block.
  • Then you need this if block again.

@@ -51,6 +51,7 @@ option(MATERIALX_BUILD_SHARED_LIBS "Build MaterialX libraries as shared rather t
option(MATERIALX_BUILD_MONOLITHIC "Build a single monolithic MaterialX library." OFF)
option(MATERIALX_BUILD_USE_CCACHE "Enable the use of ccache to speed up build time, if present." ON)
option(MATERIALX_PYTHON_LTO "Enable link-time optimizations for MaterialX Python." ON)
option(MATERIALX_PYTHON_STUBS "Enable the automated generation of MaterialX Python '.pyi' stub files through mypy's stubgen." OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is set then you need to ensure that MATERIALX_INSTALL_PYTHON is set as well
since your running stubgen off the installed package.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)
endif ()
endif()

if(MATERIALX_INSTALL_PYTHON AND PYTHON_EXECUTABLE AND NOT SKBUILD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However to include the stubs in the package itself, You need to repackage.

I think the least number of steps is

  1. run this install step to produce the locally installed MaterialX package.
  2. run your stubgen logic to get the files output to the local MaterialX folder.
  3. run this step again.

I think this should cover locally packaged plus packages generated via SKBUILD. ( I think think the wheels logic is run afterwards as an additional workflow in github actions ).

@joao-dgg
Copy link

joao-dgg commented Oct 2, 2024

I'm a little bit stuck on this. It seems like, whenever SKBUILD is being used, e.g. when building through a pip install . command, stubgen is simply not able to generate the stubs because the package itself is not installed. But the installation is, on itself, the result of the command, so it feels a bit counter intuitive?

stubgen can, for the most part, run on loose .py files as well, but not on .pyd files. For these, they need to be imported as modules (through the -m flag) or as submodules of a given package (through the -p flag).

  • It seems like the only possible way to run it for the whole package at once (through the -p flag) is if the package itself is already installed. stubgen needs to import the package in these cases and it seems like if it's not installed it simply won't work.
    • I tried to adjust the envvars and append the .pyd folders to the path to see if that would make a difference but it's not taking me anywhere.
  • If I try to process each module individually (e.g. -m PyMaterialXCore, etc) it works, but it's more error prone since we need to manually gather/parse the module files/names and that leaves a bunch of "junk" behind, like cache files and folders, which would also need to be cleaned up.

It seems like, to be able to generate the stubs on SKBUILD/ through a pip install . , we would need to somehow: build the bindings, build the wheel, install, generate the stubs, and either repackage the wheel with the stubs or rebuild it, all in one command. It seems non-trivial, in my mind, so I'm not even sure where to begin with this. I'm also concerned it could end up adding too much complexity.

Building the stubs already works, at first glance, when building through CMake and installing, without SKBUILD. For the wheels, it could be added as a new step in the workflow (build bindings, build wheels, install, generate stubs, build wheels again with the stubs included). However, I don't really see a way to do it when SKBUILD is in use, or when the bindings are not installed, at least at first glance.

One possibility, for a first version, would be to just generate the stubs when building the wheels on CI or when SKBUILD is not in use and the bindings are installed, until we find a better solution for that.

If anyone has any ideas, it would be much appreciated!

@joao-dgg
Copy link

joao-dgg commented Oct 2, 2024

Tagging @JeanChristopheMorinPerso as well, for visibility. If you can think of anything, that would be much appreciated too!

message(\"CMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}\")
if(EXISTS ${_MYPY_STUBGEN_EXECUTABLE})
execute_process(
COMMAND ${_MYPY_STUBGEN_EXECUTABLE} -p MaterialX --inspect-mode -o ${CMAKE_INSTALL_PREFIX}/python -v --ignore-errors

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if CMAKE_INSTALL_PREFIX will point at the right thing. See https://scikit-build-core.readthedocs.io/en/latest/cmakelists.html#install-directories.

@joaovbs96
Copy link
Author

joaovbs96 commented Dec 18, 2024

Hi, apologies, I haven't had time to look into this recently, but I'm planning to come back to this soon, hopefully later this week and ideally wrapping it all up early/mid January.

If memory serves me right, I ended up having issues with mypy not identifying the proper files in the right places. This issue goes away once the package has already been installed, but that doesn't help us much here. So there's two options as far as I can tell: 1) identifying a workaround for that (I might be missing the proper folder somehow?) or 2) at least when building on CI for PyPi, installing the Wheels, generating the files, and "updating" (or regenerating?) the Wheels with the stubs.

I will try 1) more for the time being, but if that still doesn't work having 2) as a "back-up" plan could be feasible, for an initial solution. I recall I was already being able to generate the stubs and place it within the Wheels when not using scikitbuild. If we end up going for the 2) approach, the only use-case that would be missing, AFAIK, would be a 'pip install .' from the repo, which we could try to add later on.

(It also seems my CLA is still not working, either. I will look into that, too.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants